Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adds support for OpenAPI 3.1 descriptions #5936

Open
wants to merge 126 commits into
base: main
Choose a base branch
from
Open

adds support for OpenAPI 3.1 descriptions #5936

wants to merge 126 commits into from

Conversation

baywet
Copy link
Member

@baywet baywet commented Dec 23, 2024

fixes #3914

depends on microsoft/OpenAPI.NET#2023

In addition to "supporting the new format" this PR does a couple of things:

  • adds early support for external references.
  • adds support for type arrays (["number", "string"]) and projects them as union types.

Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
fix: missing descriptions

Signed-off-by: Vincent Biret <[email protected]>
@baywet baywet requested a review from a team as a code owner February 14, 2025 20:13
@baywet
Copy link
Member Author

baywet commented Feb 14, 2025

and the unit tests are impacted by this bug microsoft/OpenAPI.NET#2150

andrueastman
andrueastman previously approved these changes Feb 18, 2025
Copy link
Member

@andrueastman andrueastman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏼

@baywet
Copy link
Member Author

baywet commented Feb 19, 2025

for anybody following along, we'll need to wait for the release of this fix.
microsoft/OpenAPI.NET#2157

microsoft/OpenAPI.NET#2158

@@ -19,7 +20,7 @@ internal static void InitializeInheritanceIndex(this OpenApiDocument openApiDocu
{
inheritanceIndex.TryAdd(entry.Key, new(StringComparer.OrdinalIgnoreCase));
if (entry.Value.AllOf != null)
foreach (var allOfEntry in entry.Value.AllOf.Where(static x => !string.IsNullOrEmpty(x.Reference?.Id)))
foreach (var allOfEntry in entry.Value.AllOf.OfType<OpenApiSchemaReference>())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case the Id could be null?

Suggested change
foreach (var allOfEntry in entry.Value.AllOf.OfType<OpenApiSchemaReference>())
foreach (var allOfEntry in entry.Value.AllOf.Where(static x => x is OpenApiSchemaReference xRef && !string.IsNullOrEmpty(x.Reference?.Id))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we need to maintain the OfType for the rest of the code.
So I'm going to reply with this suggestion

Suggested change
foreach (var allOfEntry in entry.Value.AllOf.OfType<OpenApiSchemaReference>())
foreach (var allOfEntry in entry.Value.AllOf.OfType<OpenApiSchemaReference>().Where(static x => x !string.IsNullOrEmpty(x.Reference?.Id))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Using a single where should offer more efficient codegen, but I don't have a strong opinion here.

Benchmark results:

Method Mean Error StdDev Allocated
RunOfTypeWhere 26.43 us 0.258 us 0.241 us 144 B
RunWhere 14.03 us 0.158 us 0.140 us 72 B

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what I meant is that line 25 we'll need to cast anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's ok. There's an extra allocation when using OfType each time this region of code is run, but it shouldn't affect much assuming this isn't a hot path. Simply casting shouldn't incur the cost of that allocation

@baywet baywet enabled auto-merge (squash) February 21, 2025 19:00
@baywet
Copy link
Member Author

baywet commented Feb 21, 2025

@andrueastman @calebkiage this is ready for final review and merge 🚀🚀🚀🚀🚀🚀

Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Add support for handling OpenAPI v3.1 definitions
4 participants